exec: forward signals to the exec session's process group - #29525
exec: forward signals to the exec session's process group#29525sahilnyk wants to merge 1 commit into
Conversation
1147aab to
2ae8cf4
Compare
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
Overall LGTM. I have small nits.
|
|
||
| logrus.Debugf("Enabling signal proxying to exec session") | ||
|
|
||
| go func() { |
There was a problem hiding this comment.
The goroutine started by ProxyExecSignals runs forever if no signal is ever sent. In the happy-path case, most exec sessions exit normally. This is fine for a CLI tool that's about to exit.
| fi | ||
| done | ||
|
|
||
| kill -TERM $kidpid |
There was a problem hiding this comment.
Maybe wait after sending signal?
2ae8cf4 to
3f87e73
Compare
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
LGTM
PTAL @podman-container-tools/podman-maintainers @podman-container-tools/podman-reviewers
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
Using a PID like that is unsafe, the pid can be reused and the goroutine is never cancelled so it might send the wrong signal to the wrong process after the main exec session exited.
While this kind of problem exists in many places throughout the codebase podman exec was already converted to using pidfds where possible. So this really must use the the getExecSessionPID and pidhandle.NewPIDHandleFromString logic to verify the pid is really want we want to send signals to.
I think design wise we should not own/act on pids outside of libpod like that. It really should have a libpod function (c *Container) ExecKill(session string, signal unit) which correctly locks the container, syncs the db state and then checks if the session still exists and only then uses the pidfd data to kill it.
then I am not sure we want different behaviours for local vs remote, I understand just doing local is easy but I think ignoring the remote client is a mistake and I rather see us adding a proper REST API to send the signal as well there to have this all in one go.
| @test "podman sigproxy test: exec" { | ||
| skip_if_remote "exec sig-proxy is not implemented for podman-remote" | ||
|
|
||
| run_podman run -d --name c_exec $IMAGE top |
There was a problem hiding this comment.
I know existing tests here also hardcode the names but please create a variable for the name cname=c-exec-$(safename), the safename is important to ensure unique per tests names. These tests could be parallelised in the future
There was a problem hiding this comment.
Fixed, using cname=c-exec-$(safename) now. Carrying this over to the new PR.
| done | ||
|
|
||
| kill -TERM $kidpid | ||
| wait $kidpid || true |
There was a problem hiding this comment.
please check the exit code of podman exec, it is important to tests that we exit with the right status number for a sigterm, and not silently treat it as success
There was a problem hiding this comment.
Added the check. Will bring this over to the new PR too.
|
@Luap99 I'll close this PR and open a new one addressing the design you described. For the remote client, should I open a separate PR to add the REST API for sending the signal, or did you have something else in mind? |
you can do that all in one PR. In general if changes are request just amened the commit and force push the the same branch. creating new PRs breaks the review workflow for us as we will be unable to follow the changes properly |
Noted.
Reopening this now, will push the latest design shortly. |
|
[NON-BLOCKING] Packit jobs failed. @podman-container-tools/packit-jobs please check. Everyone else, feel free to ignore. |
Fixes: podman-container-tools#19486 Signed-off-by: Sahil Nayak <contactsahilpnayak@gmail.com>
3f87e73 to
c69e51f
Compare
github-actions
Bot
added
the
kind/api-change
3 participants
Fixes: #19486
Killing
podman execleft the command it started running inside the container, because the signal was never passed on.It now signals the process group of the exec'd command. The kernel already puts that command and its children in one group, so there is no process tree to walk.
podman exec --no-sessionhad the same bug and is fixed too.Tested on runc and crun, rootless and rootful. The new case in
032-sig-proxy.batsfails without this change.Checklist
git commit -s). The author email must match the sign-off email address. See CONTRIBUTING.md.Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?